DEX-1270 remove id from optionEditor #412

Atticus29 · 2022-06-30T19:29:22Z

Pull request checklist:

Features and bugfixes should be PRed into the develop branch, not main
All text is internationalized
- N/A
There are no linter errors
New features support all states (loading, error, etc)
- N/A
Thanks for keeping it clean! Feel free to merge your own pull request after it has been approved - just use the "squash & merge" option.

This PR removes the unneccesary id attribute from custom filed options. This brings newly-created custom field options in line with the current behavior of migrated custom fields.
The removal of id here does not seem to affect the metadata cards for sightings or encounters.

It was decided that, because the end users will be exporting, bulk importing, and otherwise using these custom field options, they should be in control of what both the labels and values are (provided that these are unique - see DEX-1270 for that issue).

QA notes:

Old behavior when an admin tried to remove any multi-select custom field option:

Before deleting "Standing":

After deleting "Standing":

(i.e., they would all be removed)

New behavior after this PR gets merged:

Before deleting "Standing":

After deleting "Standing":

Other things to confirm:

Creating a new multi-select custom field and creating options works and persists without error
Editing (add/removing) options from the above works and persists without error
Creating a new single-select (a.k.a dropdown) custom field and creating options works and persists without error
Editing (add/removing) options from the above works and persists without error
Confirm that metadata cards for sightings or encounters on some old sightings page are not wonky as a result of this.

Emily-Ke

Looks good to me.

Found a problem when creating the options

Emily-Ke · 2022-07-01T18:57:43Z

src/pages/fieldManagement/settings/saveField/OptionEditor.jsx

@@ -42,7 +41,7 @@ export default function OptionEditor({
                flexDirection: 'column',
                marginBottom: 12,
              }}
-              key={option.id}
+              key={option.value}


Option values and/or labels cannot be guaranteed to be unique within this component. If a user clicks "Add Another Option" several times, each option looks exactly the same: [{ label: '', value: '' }, { label: '', value: '' }, // etc.]

Also, "value" is not a stable identifier for a key because whenever an option's value is updated, the key changes. This causes the value input to lose focus.

Hm...what would you recommend in this case? Just generate a uuid?

Option values and/or labels cannot be guaranteed to be unique within this component. If a user clicks "Add Another Option" several times, each option looks exactly the same: [{ label: '', value: '' }, { label: '', value: '' }, // etc.]

I think that this will at least be partially ameliorated by Jon's back end work on DEX-1270 (this makes me realize that I mis-typed and this whole FE PR is for ticket DEX-1120).

It will be partially ameliorated because we will be able to know that the values from the database are unique. However, while values are being added and edited within the dialog, it will always be possible for multiple value fields to be the same.

once the values from the database are unique, it might be sufficient to add a timestamp property to objects created on the frontend ie. { label, value, created: Date.now() }. obviously the backend objects wouldn't have these but you could use key={o?.created || o?.value}

Also, "value" is not a stable identifier for a key because whenever an option's value is updated, the key changes. This causes the value input to lose focus.

for this to be resolved we would need to make sure the value used in the keys was the backend value, not the frontend's input value that changes as typed

Ok... with the most recent changes, I believe that the only way that two options would have the same key would be if 1) they were migrated and 2) they had the same exact value.

I would argue that the behavior of deletion of any one of these resulting in the deletion of all of them is a feature rather than a bug, because it helps improve the usability of the product.

brmscheiner · 2022-07-06T17:44:10Z

src/components/Button.jsx

-      >
-        {children}
-      </Button>
+      <Tooltip title={tooltiptext}>


I know this makes things more complicated, but the Tooltip component shouldn't be rendered unless it is being used

brmscheiner · 2022-07-06T17:46:46Z

src/pages/fieldManagement/settings/saveField/OptionEditor.jsx

+              option => !option?.value || !option?.label,
+            ).length > 0
+          }
+          tooltiptext={


should be camelCase (tooltipText)

The code has been changed since the review.

Emily-Ke · 2022-07-06T23:04:09Z

src/components/Button.jsx

+    display = 'panel',
+    loading = false,
+    style,
+    disabled,
+    showTooltip = false,
+    tooltipText = '',
+    size,
+    ...rest


Anything that is being passed through can just be part of ...rest. I think you only need:

{ showTooltip = false, tooltipText = '', ...rest }

Removing everything else actually broke things. Curious.

I didn't see any errors with this:

const ButtonWithTooltip = function ( { showTooltip = false, tooltipText = '', ...rest }, ref, ) { if (showTooltip) return ( <Tooltip title={tooltipText}> <span> <CoreForwardRef ref={ref} {...rest} /> </span> </Tooltip> ); else return ( <CoreForwardRef ref={ref} {...rest} /> ); };

🤦 Thank you. Addressed in 75e6ad4

Emily-Ke · 2022-07-06T23:08:42Z

src/pages/fieldManagement/settings/saveField/OptionEditor.jsx

+          showTooltip
+          tooltipText={
+            displayedOptions.filter(
+              option => !option?.value || !option?.label,
+            ).length > 0
+              ? intl.formatMessage({
+                  id: 'UNFINISHED_OPTIONS',
+                })
+              : ''
+          }


When the options are valid, there is no need for a Tool tip.

Not sure what you're thinking here. Do you mean that it would be better to have a separate element when options are valid?

I can't pass null to a tooltip without weird behavior (it just shows an empty tooltip), but I can pass undefined. Is this what you mean?

Changed in b2c0f77

I think that showTooltip should be false. It that case tooltipText will not be used.

Ah. Thanks. Addressed in ed875d1

Emily-Ke · 2022-07-06T23:09:38Z

src/pages/fieldManagement/settings/saveField/OptionEditor.jsx

+            displayedOptions.filter(
+              option => !option?.value || !option?.label,
+            ).length > 0


It might be worth defining an areOptionsValid variable for clarity.

Changed in b2c0f77

src/pages/fieldManagement/settings/saveField/OptionEditor.jsx

Emily-Ke · 2022-07-07T22:33:51Z

src/pages/fieldManagement/settings/saveField/OptionEditor.jsx

+    filter(
+      displayedOptions,
+      option => !option?.value || !option?.label,
+    ).length === 0;


some might be easier to read here.
https://lodash.com/docs/#some

Fix in 0f31a48

Emily-Ke · 2022-07-07T22:36:39Z

src/pages/fieldManagement/settings/saveField/OptionEditor.jsx

+      option => !option?.value || !option?.label,
+    ).length === 0;
+  const areAllValuesUnique =
+    uniq(map(displayedOptions, option => option?.value)).length ===


uniqBy would let you skip the map
https://lodash.com/docs/#uniqBy

Fix in 0f31a48

Emily-Ke · 2022-07-07T22:39:04Z

src/pages/fieldManagement/settings/saveField/OptionEditor.jsx

+                  flexDirection: 'column',
+                  marginBottom: 12,
+                }}
+                key={optionIndex}


Using the index as the key feels dangerous. I agree that it works in this case though.

It was getting too complicated, and this really helped to simplify.

Emily-Ke · 2022-07-07T22:41:04Z

src/pages/fieldManagement/settings/saveField/OptionEditor.jsx

-        })}
+            );
+          },
+          [],


The signature for map does not have a third argument.
https://lodash.com/docs/#map

WHY DO I KEEP DOING THIS?

Fix in 0f31a48

DEX-1270 remove id from optionEditor

b1f5534

Atticus29 requested review from brmscheiner and Emily-Ke June 30, 2022 19:29

Atticus29 marked this pull request as ready for review June 30, 2022 19:41

Emily-Ke previously approved these changes Jun 30, 2022

View reviewed changes

Emily-Ke previously requested changes Jul 1, 2022

View reviewed changes

Atticus29 force-pushed the DEX-1120-fix-option-delete-for-multi-select branch 2 times, most recently from aaf2d88 to 47e8dcb Compare July 6, 2022 17:11

DEX-1120 respond to code review feedback

77c5ee3

Atticus29 force-pushed the DEX-1120-fix-option-delete-for-multi-select branch from 47e8dcb to 77c5ee3 Compare July 6, 2022 17:14

brmscheiner reviewed Jul 6, 2022

View reviewed changes

DEX-1120 respond to code review feedback part 2

2ffec49

Atticus29 force-pushed the DEX-1120-fix-option-delete-for-multi-select branch from 319b5df to 2ffec49 Compare July 6, 2022 22:29

brmscheiner approved these changes Jul 6, 2022

View reviewed changes

Emily-Ke suggested changes Jul 6, 2022

View reviewed changes

DEX-1120 respond to code review feedback part 3

b2c0f77

Atticus29 force-pushed the DEX-1120-fix-option-delete-for-multi-select branch from 5767ec6 to b2c0f77 Compare July 7, 2022 17:40

Atticus29 added 2 commits July 7, 2022 10:46

DEX-1120 only show tooltip when not valid

ed875d1

DEX-1120 DRY up the ButtonWithTooltip props

75e6ad4

Emily-Ke approved these changes Jul 7, 2022

View reviewed changes

DEX-1120 respond to lodash improvement feedback

0f31a48

Atticus29 merged commit ce66873 into develop Jul 7, 2022

Atticus29 deleted the DEX-1120-fix-option-delete-for-multi-select branch July 7, 2022 23:50

Emily-Ke mentioned this pull request Jul 9, 2022

Dex 761 social group site settings #419

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEX-1270 remove id from optionEditor #412

DEX-1270 remove id from optionEditor #412

Atticus29 commented Jun 30, 2022 •

edited

Loading

Emily-Ke left a comment

Emily-Ke Jul 1, 2022

Emily-Ke Jul 1, 2022

Atticus29 Jul 1, 2022

Atticus29 Jul 1, 2022 •

edited

Loading

Emily-Ke Jul 1, 2022

brmscheiner Jul 1, 2022

brmscheiner Jul 1, 2022 •

edited

Loading

Atticus29 Jul 5, 2022

brmscheiner Jul 6, 2022

brmscheiner Jul 6, 2022

Emily-Ke Jul 6, 2022

Atticus29 Jul 7, 2022

Emily-Ke Jul 7, 2022

Atticus29 Jul 7, 2022

Emily-Ke Jul 6, 2022

Atticus29 Jul 7, 2022

Atticus29 Jul 7, 2022

Emily-Ke Jul 7, 2022

Atticus29 Jul 7, 2022

Emily-Ke Jul 6, 2022

Atticus29 Jul 7, 2022

Emily-Ke Jul 7, 2022

Atticus29 Jul 7, 2022

Atticus29 Jul 7, 2022

Emily-Ke Jul 7, 2022

Atticus29 Jul 7, 2022

Atticus29 Jul 7, 2022

Emily-Ke Jul 7, 2022

Atticus29 Jul 7, 2022

Emily-Ke Jul 7, 2022

Atticus29 Jul 7, 2022

Atticus29 Jul 7, 2022

DEX-1270 remove id from optionEditor #412

DEX-1270 remove id from optionEditor #412

Conversation

Atticus29 commented Jun 30, 2022 • edited Loading

QA notes:

Old behavior when an admin tried to remove any multi-select custom field option:

New behavior after this PR gets merged:

Emily-Ke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atticus29 Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brmscheiner Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atticus29 commented Jun 30, 2022 •

edited

Loading

Atticus29 Jul 1, 2022 •

edited

Loading

brmscheiner Jul 1, 2022 •

edited

Loading